Python rust backend #470
Python rust backend #470
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Based on the provided files, I'll summarize the key changes in this pull request focusing on the most important aspects:
Major transition from C to Rust backend bindings with significant architectural changes:
- Introduced thread-safe Rust wrappers around C FFI pointers using Mutex protection in
audio_driver.rsandbackend_session.rs - Added new
integer_enum!macro inmacros.rsfor type-safe enum conversions between Rust and C - Implemented proper resource cleanup through Rust's Drop trait for audio drivers and backend sessions
- Created PyO3-based Python bindings in
shoop_py_backendmodules to expose Rust functionality - Removed global backend/driver tracking in favor of Rust's ownership model
Key concerns to address:
- Mutex lock error handling needs improvement in
audio_driver.rsandbackend_session.rs - Unsafe FFI operations require additional validation and error checks
- Memory safety guarantees around pointer handling could be strengthened
- Resource cleanup timing during application shutdown needs verification
The changes represent a significant architectural improvement but require careful review of thread safety and resource management aspects.
24 file(s) reviewed, 33 comment(s)
Edit PR Review Bot Settings | Greptile
| void destroy_audio_driver(shoop_audio_driver_t *driver) { | ||
| return api_impl<void, log_level_debug_trace>("destroy_audio_driver", [&]() { | ||
| auto _driver = internal_audio_driver(driver); | ||
| if (!_driver) { return; } |
There was a problem hiding this comment.
style: potential memory leak - should delete the driver pointer after closing and removing from active drivers
| _driver->close(); | ||
| g_active_drivers.erase(_driver); |
There was a problem hiding this comment.
style: close() should be called after removing from g_active_drivers to prevent race conditions if close triggers callbacks
| audio_obj = shoop_py_backend.MultichannelAudio(n_channels, n_frames) | ||
| resampled_obj = audio_obj.resample(target_n_frames) |
There was a problem hiding this comment.
logic: No data is being copied from input audio array to audio_obj before resampling
| QTimer.singleShot(1, lambda: self.quit()) | ||
| self.exec() |
There was a problem hiding this comment.
logic: The exec() call after scheduling quit() could cause a race condition if backends are not fully closed
| anyhow = "1.0" | ||
| static_assertions = "1.1" |
There was a problem hiding this comment.
style: Consider pinning exact version with '=1.1.1' instead of '1.1' to ensure reproducible builds
| fn unsafe_backend_ptr (&self) -> usize { | ||
| unsafe { self.obj.unsafe_backend_ptr() as usize } | ||
| } |
There was a problem hiding this comment.
logic: unsafe pointer exposure without validation or documentation of safety requirements
| fn set_audio_driver(&self, driver : &AudioDriver) -> PyResult<()> { | ||
| if self.obj.set_audio_driver(&driver.obj).is_ok() { | ||
| Ok(()) | ||
| } else { | ||
| Err(PyErr::new::<pyo3::exceptions::PyException, _>("set_audio_driver() failed")) | ||
| } | ||
| } |
There was a problem hiding this comment.
style: error message lacks detail about what specifically failed in set_audio_driver
| pub struct BackendSession { | ||
| pub obj : backend_bindings::BackendSession, | ||
| } |
There was a problem hiding this comment.
style: pub field allows direct access to backend object, bypassing safety checks
| impl MultichannelAudio { | ||
| #[new] | ||
| fn py_new(n_channels : u32, n_frames : u32) -> PyResult<Self> { | ||
| Ok(MultichannelAudio { obj: backend_bindings::MultichannelAudio::new(n_channels as u32, n_frames as u32).unwrap() }) |
There was a problem hiding this comment.
logic: Using unwrap() here is unsafe and will panic if allocation fails. Should properly handle the error case and convert to PyErr
| #[pymethods] | ||
| impl MultichannelAudio { | ||
| #[new] | ||
| fn py_new(n_channels : u32, n_frames : u32) -> PyResult<Self> { |
There was a problem hiding this comment.
logic: No validation of n_channels or n_frames being non-zero. Should check inputs are valid before allocation
No description provided.